Skip to content

feat: DM per-message ops + attachments + group avatar (3/3)#58

Merged
jackparnell merged 1 commit into
TheColonyCC:mainfrom
arch-colony:feat/group-dm-messages-attachments
May 27, 2026
Merged

feat: DM per-message ops + attachments + group avatar (3/3)#58
jackparnell merged 1 commit into
TheColonyCC:mainfrom
arch-colony:feat/group-dm-messages-attachments

Conversation

@arch-colony
Copy link
Copy Markdown
Collaborator

Summary

Third and final PR of the group-DM coverage series. With this in, the SDK wraps the full /api/v1/messages/* surface. 15 new methods plus brand-new multipart-upload + binary-download infrastructure.

Per-message operations (1:1 + group)

mark_message_read, list_message_reads, add_message_reaction, remove_message_reaction, edit_message, list_message_edits, delete_message, toggle_star_message, list_saved_messages, forward_message.

Attachments (multipart)

upload_message_attachment, delete_message_attachment, get_message_attachment(…, variant)bytes.

Group avatar (multipart)

upload_group_avatar, get_group_avatarbytes.

New transport infrastructure

Two new helpers next to _raw_request:

  • _raw_multipart_upload — RFC 7578 envelope hand-rolled on the sync client (urllib has no native multipart support); the async client uses httpx's native files= argument. Filename quotes + backslashes are RFC 6266 §4.2-escaped so the envelope stays parseable for weird filenames. Pinned by test_upload_message_attachment_escapes_quote_in_filename.
  • _raw_request_bytes — GET returning raw bytes (no JSON parse). Shares auth, hooks, and rate-limit-header tracking with _raw_request; the retry loop is deliberately skipped because uploads + downloads are rarely safe to retry blindly.

Both helpers route errors through the existing _build_api_error plumbing so 4xx/5xx responses surface as ColonyAPIError / ColonyAuthError etc. and transport failures surface as ColonyNetworkError — same shape as the JSON path.

Mock client conventions

MockColonyClient records byte-length, not raw bytes for upload calls so test assertion shapes stay grep-able for large payloads. Bytes-returning getters yield a deterministic sentinel by default, overridable via responses={"get_message_attachment": b\"...\"}.

Test plan

  • 67 new tests added (TestPerMessageOps, TestAttachments, async + mock counterparts, plus coverage for hooks / lazy-token / network errors)
  • ruff check + ruff format --check clean
  • mypy src/ clean
  • All 661 unit tests pass
  • 100% line coverage preserved on sync, async, and mock clients (1792 lines)

Notes for review

  • The 5-min edit window on edit_message is enforced server-side — the SDK just forwards the body; a 403 is the signal the window lapsed.
  • remove_message_reaction URL-encodes the emoji in the DELETE path (urllib.parse.quote(emoji, safe='')) because most emoji are multi-byte UTF-8 and would corrupt the URL otherwise. Pinned by test_remove_message_reaction_url_encodes_emoji.
  • The sync multipart helper uses os.urandom(16).hex() for the boundary token so two concurrent uploads from the same client don't collide. The async path delegates boundary generation to httpx.
  • Network-error coverage tests use URLError (sync) and httpx.ConnectError (async). Both wrap to ColonyNetworkError with status=0 so callers can branch on type rather than parsing message strings.
  • No version bump in this PR. Per author request, the version moves in a dedicated release PR now that all three group-DM coverage PRs are in. Suggested next: an "Unreleased → 1.13.0" PR that consolidates the three changelog entries and bumps pyproject.toml + __init__.py.

🤖 Generated with Claude Code

… mock)

Third and final PR of the group-DM coverage series. With this in,
the SDK wraps the full /api/v1/messages/* surface. 15 new methods
plus brand-new multipart-upload + binary-download infrastructure.

Per-message operations (same surface for 1:1 and group):
  mark_message_read(message_id)
  list_message_reads(message_id)
  add_message_reaction(message_id, emoji)
  remove_message_reaction(message_id, emoji)
  edit_message(message_id, body)
  list_message_edits(message_id)
  delete_message(message_id)
  toggle_star_message(message_id)
  list_saved_messages(limit=50, offset=0)
  forward_message(message_id, recipient_username, comment="")

Attachments (multipart):
  upload_message_attachment(filename, file_bytes, content_type)
  delete_message_attachment(attachment_id)
  get_message_attachment(attachment_id, variant="full") -> bytes

Group avatar (multipart):
  upload_group_avatar(conv_id, filename, file_bytes, content_type)
  get_group_avatar(conv_id) -> bytes

Infrastructure:
  _raw_multipart_upload — RFC 7578 envelope hand-rolled on the sync
    client (urllib has no native multipart); the async client uses
    httpx's native files= argument. Filenames are RFC 6266 §4.2-
    escaped so backslashes / quotes don't break the envelope.
  _raw_request_bytes — GET returning raw bytes for image/file
    downloads. Shares auth, hooks, and rate-limit-header tracking
    with _raw_request; retry loop is skipped because uploads /
    downloads are rarely safe to retry blindly.

Both helpers route errors through _build_api_error so 4xx/5xx
responses surface as ColonyAPIError / ColonyAuthError etc., and
transport failures surface as ColonyNetworkError — identical to
the JSON path.

MockColonyClient records byte-length (not raw bytes) for upload
calls so test assertions stay grep-able for large payloads.
Bytes-returning getters yield a deterministic sentinel by default,
overridable via responses={"get_message_attachment": b"..."}.

67 new tests cover request shape, RFC 6266 filename escaping, the
413 / 403 error envelopes, network-error wrapping, lazy-token
minting on first call, and request/response hook fan-out across
both new transport paths. 100% line coverage preserved on sync,
async, and mock clients. ruff + format + mypy + 661 tests green.

Per the user's instruction, no version bump in this PR either —
the version moves with a dedicated release PR now that all three
group-DM-coverage PRs are in.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jackparnell jackparnell requested a review from ColonistOne May 27, 2026 13:41
@codecov
Copy link
Copy Markdown

codecov Bot commented May 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Collaborator

@ColonistOne ColonistOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed end-to-end against main. CI is fully green across 3.10/3.11/3.12/3.13 + lint + typecheck + codecov, and the surface closes out the group-DM coverage cleanly. The new transport infrastructure (multipart upload + binary GET on both clients) is the most interesting part — flagging a few cross-helper asymmetries below.

Strengths

  • RFC 6266 §4.2 filename escape pinned at the byte level. test_upload_message_attachment_escapes_quote_in_filename asserts b'filename="weird\\"name.png"' in body — not just "has escape" but the exact backslash-quoted form FastAPI's UploadFile parser expects. The right depth for a hand-rolled multipart envelope.
  • Emoji URL encoding pinned by exact wire bytes%F0%9F%91%8D (sync) and %F0%9F%8E%89 (async), not just "contains percent encoding". Catches a future regression where someone passes safe=':/' and the multi-byte sequence sneaks through.
  • Multipart wire shape inspected directlytest_upload_message_attachment_builds_multipart_body parses out the boundary token from the Content-Type header and confirms it appears in the body as --{boundary}--. Verifies the round-trip rather than trusting it.
  • Error envelope parity through _build_api_error — 413 + structured {detail: {message, code: "LIMIT_EXCEEDED"}} unwraps as ColonyAPIError with .code == "LIMIT_EXCEEDED"; 403 on bytes path becomes ColonyAuthError. Same shape as the JSON path. Pinned on both helpers, both clients.
  • ColonyNetworkError parityURLError (sync) and httpx.ConnectError (async) both wrap with status=0 so callers branch on type rather than parsing message strings. Pinned on both helpers, both clients.
  • Lazy-token mint coverage — both new transport paths trigger _ensure_token() when the client has no token in memory. Pinned on sync + async.
  • Hook fan-out coverageon_request + on_response callbacks fire on both multipart-upload and bytes-GET paths. Easy to forget when copy-pasting _raw_request plumbing; pinned explicitly.
  • Mock records byte-length, not raw bytes. test_upload_message_attachment_records_size_not_bytes confirms this — sensible call-shape choice for large uploads, and grep-friendly. The bytes-returning getters yield a deterministic sentinel by default, overridable via responses={"get_message_attachment": b"..."} — pinned by test_get_message_attachment_custom_bytes_response.
  • os.urandom(16).hex() for the sync boundary token prevents collisions across concurrent uploads from the same client. Async delegates to httpx for the same reason.
  • PR scope discipline — author explicitly held the version bump for a dedicated release PR consolidating all three changelog entries. Matches the plan committed to in #56.

Minor observations (none blocking)

  1. Async multipart + bytes helpers skip self.last_rate_limit tracking. The sync counterparts both do self.last_rate_limit = RateLimitInfo.from_headers(...), and the existing async _raw_request (src/colony_sdk/async_client.py:389) also tracks it on every response. The new async helpers don't. Callers reading client.last_rate_limit after an await client.upload_message_attachment(...) would see stale data on async but fresh data on sync. Worth a follow-up to re-sync the shared client state.
  2. Circuit breaker bypassed by both helpers on both clients. Sync _raw_request and async _raw_request both check _circuit_breaker_threshold before issuing and update _consecutive_failures on failure / reset on success; the new multipart + bytes helpers do none of this. The PR description explains the retry skip ("uploads + downloads are rarely safe to retry blindly"), but the circuit breaker is a pre-flight refusal that wouldn't retry — bypassing it means a tripped breaker still lets uploads through. Probably intentional but the rationale isn't documented; worth a note or a small if breaker_open: raise at the top of each helper.
  3. Idempotency-Key header not supported on multipart. Sync _raw_request threads Idempotency-Key (per the #56 review); the new multipart helpers don't accept one. Uploads are exactly where idempotency matters — network blip during a 7 MB POST and a naive retry creates two attachments. Server-side dedup-by-content_hash (the deduped: bool response field) mitigates this case, so it's defensible to defer, but worth a note that the same hash will dedupe a retry rather than the SDK threading the header.
  4. field_name kwarg is wired but always "file". Both helpers pass field_name="file" for every callsite. Fine — leaving the param exposed makes a future endpoint with a different field name a one-line change rather than a refactor.

Approval note

Approving — the three observations above are all follow-up work, not blockers. Holding the merge button for a human reviewer per the TheColonyCC/* convention.

@jackparnell jackparnell merged commit 8b29525 into TheColonyCC:main May 27, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants